Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix hydration in solid renderer #8365

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

ryansolid
Copy link
Contributor

@ryansolid ryansolid commented Sep 1, 2023

Changes

I was playing on stream and noticed top-level fragments weren't working properly in Solid Astro integration. Duplicated elements and other weirdness. On further exploration I realized this is because the renderer always creates new DOM nodes by cloning so it can't match them.

I then noticed that the hydration check wasn't working because it was being done before any hydrate calls were being done top-level. This meant it was always false so we were always creating the nodes as if client rendered as well.

Also hierarchical children weren't working as we'd consume all descendant slots and they'd override the parent. I'm hoping this treewalker is fine for performance but if you have ideas open to that.

The solution might seem a bit odd but given visibility changes (ie.. islands controlling when to render slots) I think that creating vs consuming isn't an either/or like it was before but per slot. We try to fill from the DOM first and failing that we fill out the templates. I suppose we could make it do that lazily for even more performance but them prop handling needs some more work.

Testing

Honestly I only tested this in my Hackernews project which let me test recursion and visibility show and hide. But I didn't test client only or other modes so there could be gaps.

Docs

This shouldn't require docs changes. The API is the same. It just fixes a bunch of random bugs and should improve performance for the shown element case.

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2023

🦋 Changeset detected

Latest commit: ae4ef71

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels Sep 1, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ryansolid! I'll give this a more thorough test run, but the code looks totally reasonable.

🙌 Appreciate you jumping on this

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! No notes, this looks great. Merging as-is.

@natemoo-re natemoo-re merged commit a525d5d into withastro:main Sep 6, 2023
4 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants